FIX: Remove unused decimal separator code and add regression tests#384
Conversation
|
@microsoft-github-policy-service agree |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. 📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.hpp: 58.8%
mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.row.py: 66.2%
mssql_python.helpers.py: 67.5%
mssql_python.pybind.ddbc_bindings.cpp: 69.4%
mssql_python.pybind.ddbc_bindings.h: 69.7%
mssql_python.pybind.connection.connection.cpp: 73.6%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 79.6%
mssql_python.connection.py: 84.1%🔗 Quick Links
|
bewithgaurav
left a comment
There was a problem hiding this comment.
Thank you @Chetan-Kansal for the PR!
Looking closely with the codebase, the core issue #295 was already fixed in PR #320, which introduced this comment in the code at lines 3142-3147:
// Always use standard decimal point for Python Decimal parsing
// The decimal separator only affects display formatting, not parsing
py::object decimalObj = PythonObjectCache::get_decimal_class()(py::str(cnum, safeLen));This ensures DECIMAL values are parsed correctly regardless of the setDecimalSeparator() setting.
However, your PR performs important cleanup and adds a focused regression test:
- Removes unused dead code: The
decimalSeparatorvariables at lines 2902 and 3625 were declared but never used - Adds regression tests: Ensures the fix stays in place and prevents future regressions
Suggested Title Change
Since this is cleanup + testing rather than the actual fix, I suggest updating the PR title to be more accurate:
Current: "FIX: clarify decimal parsing in bulk fetch and add regression test"
Suggested: "FIX: Remove unused decimal separator code and add regression tests"
- The code changes look correct (removing dead code)
- The tests are valuable (preventing regression)
Great work on the PR and adding comprehensive tests! This will help ensure the fix remains stable.
|
Hi Gaurav Sir, |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
bewithgaurav
left a comment
There was a problem hiding this comment.
lgtm, thanks for contributing!
Work Item / Issue Reference
GitHub Issue: #295
Summary
Removes misleading unused decimal-separator code from the bulk fetch path and adds a regression test to ensure DECIMAL values are parsed correctly regardless of setDecimalSeparator().